The possibility to specify a path to the settings file #41

kudashevs · 2022-02-16T12:32:41Z

Hello again 👋,

This is a draft PR of the first step of a proposition to the laravel-ignition.

This is a new component FileConfigManager which is implemented through a new ConfigManager interface. The interface is a little bit over-engineered with the getSource method, however, I've found it useful for testing purposes and it could be useful in the future (but it could be removed, though). So, the new FileConfigManager gets all the responsibility to manage the settings file:

it accepts a path through the constructor and validates it
if the path is not provided it tries to find an appropriate path from the environment (was taken from DefaultConfigFinder)
it can load the options from a file
it can save the options to the file
it can create the file (and this seems the most important because DefaultConfigFinder didn't do that, so DefaultConfigFinderTest didn't work properly for me if I didn't have a created settings file)
it is covered with tests in some weak points

AlexVanderbist

Great PR, I've commented on a couple of nitpicks but nothing major! Thanks for taking the time to add proper tests as well. Looking forward to merging this one 👍

AlexVanderbist · 2022-02-17T21:12:05Z

src/Config/FileConfigManager.php

+
+class FileConfigManager implements ConfigManager
+{
+    private const SETTINGS_FILE_NAME = '.ignition.json';


Maybe we can rename this to CONFIG_FILE_NAME to keep it in line with the ConfigManager, etc...

Suggested change

private const SETTINGS_FILE_NAME = '.ignition.json';

private const CONFIG_FILE_NAME = '.ignition.json';

Hello,

I kind of agree, but to me, the config/ignition.php is a config file and .ignition.json is a settings file. One of the reasons for this separation is the Laravel Ignition button naming (we have a Save settings button there). So, not to confuse users I tried to differentiate these two concepts by introducing this term settings file. That's why I would like to stick to this name settings file and use it for this file if you don't mind.

However, if you don't like this differentiation at all let's change it. @AlexVanderbist please let me know about your decision.

src/Config/FileConfigManager.php

AlexVanderbist · 2022-02-17T21:22:26Z

src/Config/FileConfigManager.php

+        return rtrim($path, DIRECTORY_SEPARATOR);
+    }
+
+    public function createSource(): bool


createSource might be a bit vague. Maybe we can rename it to something a bit more explicite?

Suggested change

public function createSource(): bool

public function createConfigFile(): bool

I totally agree that the term source is vague. However, I tried to make the contract as abstract as possible (in case if in the future somebody is going to use a database or other persistent source to keep the settings). If there is no need for the abstraction let's rename it according to your final decision about the config or settings file above. But if there is a chance that the persistent source will have changed in the future, let's try to come up with a less vague definition for that (for example Storage).

@AlexVanderbist please let me know about your decision.

AlexVanderbist · 2022-02-17T21:24:09Z

src/Config/FileConfigManager.php

+        return rtrim($path, DIRECTORY_SEPARATOR);
+    }
+
+    public function createSource(): bool


Also, what do you think about calling this method from the save() method? We can still keep it public for use inside of tests tho.

The idea to introduce and make createSource() method public was to give the possibility to create a settings storage somewhere outside. For example, if I don't want to use the createSource every time on the saving operation, I can just use it in a ServiceProvider to create it once. So, I'm still thinking about how to implement it and you are absolutely right that it's better to remove this call from IgnitionConfig.php.

I need to think about a better solution on how to create storage once and not use it on every save() call. Until then, I'll move it to the save() method.

AlexVanderbist · 2022-02-17T21:25:37Z

src/Config/IgnitionConfig.php

+        if (! function_exists('app')) {
+            return new FileConfigManager();
+        }
+
+        return app(ConfigManager::class);


Interesting approach! :D

Frankly speaking, this approach is a little bit dangerous, because it leads to an exception if there is no bound instance in the container (according to PSR-11 and according to the Larave documentation). In my view, this is the case where fail fast looks better than fail soft.

AlexVanderbist · 2022-02-17T21:26:04Z

src/Config/IgnitionConfig.php

-        } catch (Throwable) {
-            return false;
-        }
+        $this->manager->createSource();


Can this call be moved to inside of the $manager->save() method?

AlexVanderbist · 2022-02-17T21:27:55Z

src/Contracts/ConfigManager.php

+    /** @return array<string, mixed> */
+    public function load(): array;
+
+    /** @param array<string, mixed> $options */
+    public function save(array $options): bool;
+
+    /** @return array<string, mixed> */
+    public function getSource(): array;


I don't think these doc block typehints add a lot (I know we've used them in other places as well and I should really go over those and clean up 😅)

I would rather leave them. At least, they show that they are expected to be associative arrays :)

tests/Config/FileConfigManagerTest.php

…tant.

…generate method.

…ilepath method.

…ws environment test.

…erate filepath method.

…indows environment test.

… filepath method.

…te filepath method.

…ttings file.

…e tests.

…punit built-in method.

…e in the non-windows environment.

…e in the windows environment.

…k in some tests.

…llFilePath method.

…dFromFile method.

…eToFile method.

…eFullFilePath method.

…FromArgument method.

…empty path.

…ateSource method.

… call from some tests.

…t method.

…method.

…nvironment method.

…ethod.

…ile test.

…initConfigManager method.

kudashevs · 2022-02-20T00:00:11Z

@AlexVanderbist I tried to push it gently but without any luck. Sorry for that 😭 I should've made another PR from another branch, probably. I don't know how to cope with such situations when the only option is to push with force. What are your suggestions, btw?

So, this is a modified solution reworked by taking into account suggestions and some new ideas:

Maybe we can rename this to CONFIG_FILE_NAME to keep it in line with the ConfigManager, etc...
Because I haven’t received any feedback on the explanations about my decision, I would like to stick to this naming for now.
For most OSS package we avoid private for all properties and functions
I changed the visibility for all the methods which look worth being inherited.
Explicitly importing Throwable is more in line with Laravel's codebase
Done for all the cases.
createSource might be a bit vague. Maybe we can rename it to something a bit more explicit?
The interface of the ConfigManager was change to:

  - load() 
  - save()
  - createPersistent() // could be safely removed if it seems a good idea not to expose it to the outside world
  - getPersistentInfo()

, that looks better to me and more self-explantory

Also, what do you think about calling this method from the save() method?
Done. The FileConfigManager uses a new flow. It creates a file at the initialization moment (createFile()) and retains two states ($path, $file) for the future use. That way it is not still necessary to create a persistent storage somewhere else (that's why createPersistent() method could be removed from the public Interface if it seems a good idea).
Interesting approach!
The approach turned out a fig failure when I had tried to update laravel-ignition with this package. So, the solution was rethought and modified. Now, it should work without any possible issues.
Can this call be moved to inside of the $manager->save() method?
Done. It was mentioned above.
I don't think these doc block typehints add a lot
Because I haven’t received any feedback I left them as is.

Besides these refinements, I have finished a branch with the integration of these changes to the laravel-ignition. I haven't made a PR yet, because the branch uses a non-standard composer.json. Could you please give it a look?

As an afterthought, I would like to propose to extract the functionality based on user's home directory (everything under the initPathFromEnvironment()) to another class. It's not really crucial, but it is obviously sort of my fault, that I mixed them up. It seems like these (use predefined filename vs use the dynamically retrieved user home directory) are two different responsibilities with different functionality (works per project vs works for all the projects). So, it is better to do in the future.

kudashevs · 2022-02-26T21:21:41Z

I'd better close it.

AlexVanderbist · 2022-03-01T10:35:02Z

Hey @kudashevs, sorry about the delay. I didn't mean to come across as disrespectful or uninterested. If you would still like to see this PR merged, feel free to re-open it and I'll be happy to continue looking at it.

kudashevs · 2022-03-01T22:43:53Z

Hello @AlexVanderbist

No worries. I just thought that the proposition is not still interesting (because it turned out to be kind of messy, big, and complicated). Let's try it one more time. If you don't mind I'm going to open a new PR.

AlexVanderbist · 2022-03-02T10:40:50Z

Awesome, thanks! I'll go over it later today 👍

AlexVanderbist requested changes Feb 17, 2022

View reviewed changes

kudashevs added 29 commits February 18, 2022 19:56

Refactor DefaultConfigFinder extract the settings file name to a cons…

4c57a85

…tant.

Refactor DefaultConfigFinder extract a generate file path method.

abb1e36

Refactor DefaultConfigFinder upfate filepath generation with the new …

cdf4ed0

…generate method.

Update DefaultConfigFinder add constructor with a path parameter.

a95370b

Update DefaultConfigFinder add filepath generator to the get config f…

895fdf2

…ilepath method.

Update DefaultConfigFinderTest add check config finder works in Windo…

88871bd

…ws environment test.

Update DefaultConfigFinder add create a file functionality to the gen…

c13ff86

…erate filepath method.

Update DefaultConfigFinderTest add check config finder works in non-W…

c9ccd6f

…indows environment test.

Refactor DefaultConfigFinder rename the path variable in the generate…

964237e

… filepath method.

Refactor DefaultConfigFinder move the is_readable check to the genera…

baf4434

…te filepath method.

Refactor DefaultConfigFinder extract format home directory method.

056483e

Update DefaultConfigFinderTest add retrieve settings filepath helper.

ff43fa6

Update DefaultConfigFinderTest add teardown with the deletion of a se…

5e017ff

…ttings file.

Update DefaultConfigFinderTest add stricter testing conditions to som…

cfa485c

…e tests.

Style DefaultConfigFinderTest remove a redundant new line.

c669128

Fix DefaultConfigFinderTest fix wrong skip conditions in some tests.

8210d3c

Update DefaultConfigFinderTest refine the Windows OS check.

8e94502

Refactor DefaultConfigFinder extract the isWindows method.

2608520

Style DefaultConfigFinderTest fix the arrow functions style.

b2b94cf

Update Contracts add a new ConfigManager abstraction.

f3deef3

Update Config add FileConfigManager class with basic realization.

1dd9863

Update FileConfigManager add the source creation functionality.

7b7b475

Update FileConfigManager update Throwable in catch with globals scope.

ea9a412

Refactor FileConfigManager extract a preparePath method.

1498e67

Refactor FileConfigManagerTest change the file exists check to the ph…

f02a565

…punit built-in method.

Update FileConfigManager add initialize file path from a home variabl…

a500c83

…e in the non-windows environment.

Update FileConfigManager add initialize file path from a home variabl…

afe9928

…e in the windows environment.

Refactor FileConfigManagerTest remove redundant not empty return chec…

271e6b9

…k in some tests.

Update IgnitionConfig add a manager field with initilization.

246ade2

kudashevs added 21 commits February 18, 2022 19:56

Update FileConfigManager add a formatting of a path to the generateFu…

6d24482

…llFilePath method.

Refactor FileConfigManager move the guard clause from load to the rea…

4f030e3

…dFromFile method.

Refactor FileConfigManager move the guard clause from save to the sav…

6d42df2

…eToFile method.

Update FileConfigManager remove formatting of a path from the generat…

2299d55

…eFullFilePath method.

Update FileConfigManager add a formatting to a return of the initPath…

6eb88a7

…FromArgument method.

Update FileConfigManagerTest add check it cannot create file when an …

5dfbbcb

…empty path.

Update FileConfigManager add a guard clause for empty path in the cre…

9284fb3

…ateSource method.

Refactor FileConfigManagerTest remove no longer used the createSource…

dfec1e7

… call from some tests.

Refactor ConfigManager rename the createSource to the createPersisten…

1f71719

…t method.

Refactor ConfigManager rename the getSource to the getPersistentInfo …

e2e63fa

…method.

Refactor ConfigManager reorder interface method signatures.

92caef5

Refactor FileConfigManager reorder class methods.

f4ceb0c

Update IgnitionConfig add more checks to the isInContainer method.

c566033

Refactor FileConfigManager rename isValidFile to isFileValid method.

ac37f0b

Refactor FileConfigManager change the visibility of the initPathFromE…

2f710c6

…nvironment method.

Refactor FileConfigManager reorder class methods.

d7025fe

Refactor FileConfigManager change the visibility of the preparePath m…

672616e

…ethod.

Update IgnitionConfigTest update a title of the check retrieve from f…

0246027

…ile test.

Update IgnitionConfig remove the isInContainer method.

3315661

Update IgnitionConfig change the container is available check in the …

fcd27af

…initConfigManager method.

Refactor FileConfigManager rename the generateFullFilePath method.

b5a73c3

kudashevs requested a review from AlexVanderbist February 20, 2022 00:08

kudashevs closed this Feb 26, 2022

kudashevs deleted the feature/specify_settings_file branch February 26, 2022 21:31

kudashevs mentioned this pull request Mar 1, 2022

The possibility to specify a path to the settings file #57

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The possibility to specify a path to the settings file #41

The possibility to specify a path to the settings file #41

kudashevs commented Feb 16, 2022 •

edited

Loading

AlexVanderbist left a comment

AlexVanderbist Feb 17, 2022

kudashevs Feb 17, 2022

AlexVanderbist Feb 17, 2022

kudashevs Feb 17, 2022 •

edited

Loading

AlexVanderbist Feb 17, 2022

kudashevs Feb 17, 2022

AlexVanderbist Feb 17, 2022

kudashevs Feb 17, 2022 •

edited

Loading

AlexVanderbist Feb 17, 2022

AlexVanderbist Feb 17, 2022

kudashevs Feb 17, 2022 •

edited

Loading

kudashevs commented Feb 20, 2022 •

edited

Loading

kudashevs commented Feb 26, 2022 •

edited

Loading

AlexVanderbist commented Mar 1, 2022

kudashevs commented Mar 1, 2022 •

edited

Loading

AlexVanderbist commented Mar 2, 2022

	private const SETTINGS_FILE_NAME = '.ignition.json';
	private const CONFIG_FILE_NAME = '.ignition.json';

	public function createSource(): bool
	public function createConfigFile(): bool

The possibility to specify a path to the settings file #41

The possibility to specify a path to the settings file #41

Conversation

kudashevs commented Feb 16, 2022 • edited Loading

AlexVanderbist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kudashevs Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kudashevs Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kudashevs Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

kudashevs commented Feb 20, 2022 • edited Loading

kudashevs commented Feb 26, 2022 • edited Loading

AlexVanderbist commented Mar 1, 2022

kudashevs commented Mar 1, 2022 • edited Loading

AlexVanderbist commented Mar 2, 2022

kudashevs commented Feb 16, 2022 •

edited

Loading

kudashevs Feb 17, 2022 •

edited

Loading

kudashevs Feb 17, 2022 •

edited

Loading

kudashevs Feb 17, 2022 •

edited

Loading

kudashevs commented Feb 20, 2022 •

edited

Loading

kudashevs commented Feb 26, 2022 •

edited

Loading

kudashevs commented Mar 1, 2022 •

edited

Loading